Validate AVE record string formats - #130
Conversation
|
This is an excellent first PR, and worth being direct about why: it doesn't just implement what #125 asked for, it corrects something #125 itself got wrong. That issue assumed rfc3986-validator was sufficient, reasoning from the crosswalk schema's date format check in #121, but that was scoped to a different format string than the record schema's date-time. Catching that rfc3986-validator alone doesn't cover it, and that rfc3339-validator specifically is the missing piece, is the kind of thing that only comes from actually running the check rather than assuming an adjacent finding transfers. Good catch, genuinely. Traced the diff directly: build_validator() is a clean, purposeful extraction, it's what makes the regression test possible at all rather than being buried in main(). The test itself is correctly targeted, a deliberately malformed date-time value, asserting the real error text contains it, not just that validation fails for some reason. AVE-2026-99999 as the fixture ID is a sensible, safely non-colliding choice. All 70 real records still passing is the right thing to have checked, confirms no regression against the actual corpus, not just a clean test in isolation. Appreciate you checking the crosswalk validator too even though this PR doesn't touch it. Merging. And updating #125's own body to correct the dependency assumption, since it was wrong and this PR is the reason we now know that. |
|
hello @mmaxjr , please check the failed workflow on the test. once it fixed, we can merge. |
…st_validate_data.py PR #130 added tests/test_validate_data.py with 'from scripts import validate_records'. scripts/ deliberately has no __init__.py (this repo isn't a package, see the packages = [] note in pyproject.toml). That import resolves fine under 'python -m pytest' (which inserts cwd onto sys.path), the exact command PR #130's own verification section used, but CI's actual invocation is bare 'pytest tests/ -x -q' (.github/workflows/tests.yml), which does not. Confirmed directly: PR #130's merge commit has no tests.yml check run recorded at all, only CodeQL/dependency-graph, so this was never actually verified against CI's real invocation before merging. Fix: pythonpath = ['.'] under [tool.pytest.ini_options], the standard pytest 7+ mechanism for exactly this case. Verified with the literal CI command: pytest tests/ -x -q -> 293 passed.
2 participants
Summary
Draft202012Validatorformat checking inscripts/validate_records.pyrfc3339-validatoras a dev dependency sodate-timeformats are actually enforcedpublishedmetadataRoot cause
validate_records.pybuiltDraft202012Validator(schema)without a format checker. AddingFORMAT_CHECKERalone registersuribecauserfc3986-validatoris already present, but this environment showeddate-timewas still not registered untilrfc3339-validatorwas installed.Closes #125.
Verification
python -m pytest -q-> 281 passedpython scripts/validate_records.py-> all 70 records validpython scripts/validate_crosswalks.py-> 4/4 crosswalks validpython -m py_compile scripts/validate_records.py tests/test_validate_data.py-> passed